Asdf-Support by ViciousEagle03 · Pull Request #708 · sunpy/ndcube

ViciousEagle03 · 2024-05-20T15:54:54Z

This PR adds ASDF support for ndcube objects.

This Pull Request is currently a Work In Progress and will be continuously updated as we expand support for serializing ndcube objects to ASDF.

braingram

Thanks for sharing your work. It looks like a great start! I left a bunch of comments. I did not look over the ndcube.py changes.

braingram

I'm a bit confused. Several of the suggestions in my previous comments were marked as resolved with responses that gave me the impression they were "ok" yet the changes were not made. I was going through updating comments but stopped when I realized this PR might not be up-to-date.

~~Are there changes not committed that have the previous suggestions? If not, is there a reason these suggestions were dismissed?~~

EDIT: I'm not sure what's up with github but me posting this comment caused the PR to update for me :-/ I'm now seeing the changes made in cd31d95 which I did not previously see.

braingram

Now that I'm looking at the newest code 🤦

Just a few comments/questions and one request.

Would you add asdf to the dependencies and set a version that makes the oldestdeps job happy?

ViciousEagle03 · 2024-07-11T06:23:20Z

Now that I'm looking at the newest code 🤦

Just a few comments/questions and one request.

Would you add asdf to the dependencies and set a version that makes the oldestdeps job happy?

Sure, I am on it.

braingram

Looks good to me. Thanks!

Cadair

This PR looks amazing @ViciousEagle03 thanks for all your work on it. I have a few small-ish comments and then I think we can merge it.

Cadair · 2024-07-18T09:46:23Z

+        extra_coords._mapping = node.get("mapping")
+        extra_coords._lookup_tables = node.get("lookup_tables", [])
+        extra_coords._dropped_tables = node.get("dropped_tables")
+        extra_coords._ndcube = node.get("ndcube")


@braingram @ViciousEagle03 is asdf smart enough to handle this circular reference and not save out the ndcube object twice? i.e. I save an NDCube which has an ExtraCoords which references the same NDCube?

Theoretically yes but I believe the converter will need to be updated.
https://asdf.readthedocs.io/en/latest/asdf/extending/converters.html#reference-cycles
That seems like a good test case.

@braingram, for the current implementation I believe it is still not storing the NDCube object twice, right?
And also in the current from_yaml_tree method, it doesn't have yield but it can still deserialize the extra_coords object, that is, when the extra_coords converter is called the extra_coords.ndcube shows _PendingValue and when it gets picked up in the NDCube converter and the ndcube.extra_coords is initialized the reference to the ndcube is properly mapped in the ndcube.extra_coords._ndcube.

We can see the deserialized NDCube object and the NDCube reference address for the ExtraCoords associated is the same for the below.

(Pdb) ndcube.extra_coords._ndcube <ndcube.ndcube.NDCube object at 0x7c84fe60c2d0> NDCube ------ Shape: (10, 10) Physical Types of Axes: [('em.energy', 'time', 'pos.eq.ra', 'pos.eq.dec'), ('time', 'custom:CUSTOM')] Unit: None Data Type: float64 (Pdb) ndcube <ndcube.ndcube.NDCube object at 0x7c84fe60c2d0> NDCube ------ Shape: (10, 10) Physical Types of Axes: [('em.energy', 'time', 'pos.eq.ra', 'pos.eq.dec'), ('time', 'custom:CUSTOM')] Unit: None Data Type: float64 (Pdb)

Thanks for testing this out. This looks to be working because __set__ overwrites _ndcube when _extra_coords is assigned:

ndcube/ndcube/ndcube.py

Line 321 in 09a83f1

value._ndcube = obj

I am not sure what's going on with __set__. @Cadair is there ever an instance where:

the extra_coords attached to a NDCube will reference a different NDCube?

extra_coords will exist without a NDCube?

the extra_coords attached to a NDCube will reference a different NDCube?

No, that should be explicitly prohibited by the descriptor.

extra_coords will exist without a NDCube?

I think this might be possible yes.

extra_coords will exist without a NDCube?

I think this might be possible yes.

Is there a minimal example that would show this? Perhaps it can be used for a test case for the converter.

I think you can just instantiate one standalone and then add some coordinates to it with the same API as you can if it's attached to the cube.

turns out you can't: #753

braingram · 2024-07-19T11:21:34Z

One other minor request. Would you rename the asdf directory to _asdf (to make it more explicit that this is private API)?

ViciousEagle03 · 2024-08-04T20:34:27Z

Apologies for the delay, @nabobalis would you mind taking a look? As I mentioned earlier, the oldestdeps is failing.

nabobalis · 2024-08-04T21:03:18Z

Apologies for the delay, @nabobalis would you mind taking a look? As I mentioned earlier, the oldestdeps is failing.

It is creating a warning which is failing the CI:

AsdfWarning: Converter handles multiple tags for this extension, but does not implement a select_tag method. This previously worked because Converter subclasses inherited the now removed select_tag. This will be an error in a future version of asdf

I would just ignore the warning being created by adding it to the pytest.ini list.

ViciousEagle03 · 2024-08-05T05:57:03Z

Apologies for the delay, @nabobalis would you mind taking a look? As I mentioned earlier, the oldestdeps is failing.

It is creating a warning which is failing the CI:

AsdfWarning: Converter handles multiple tags for this extension, but does not implement a select_tag method. This previously worked because Converter subclasses inherited the now removed select_tag. This will be an error in a future version of asdf

I would just ignore the warning being created by adding it to the pytest.ini list.

Hey @nabobalis, thanks for the help. Even after adding the asdf.exceptions.AsdfWarning, some of the tests are still failing due to AssertionError, as the pixel_shape serialization of gWCS was added in version 0.20. I wonder if it might be simpler to skip the tests if the gWCS version is <0.20, rather than adding asdf as an optional dependency.

nabobalis · 2024-08-05T06:25:05Z

Hey @nabobalis, thanks for the help. Even after adding the asdf.exceptions.AsdfWarning, some of the tests are still failing due to AssertionError, as the pixel_shape serialization of gWCS was added in version 0.20. I wonder if it might be simpler to skip the tests if the gWCS version is <0.20, rather than adding asdf as an optional dependency.

Yeah that makes sense. If there is no functional way to do pixel_shape serialization until this version of gwcs, skipping them for versions older than 0.20 is to me the best way.

Cadair

This looks great to me. All my comments are minor.

Cadair · 2024-08-05T10:08:50Z

+        extra_coords._mapping = node.get("mapping")
+        extra_coords._lookup_tables = node.get("lookup_tables", [])
+        extra_coords._dropped_tables = node.get("dropped_tables")
+        extra_coords._ndcube = node.get("ndcube")


the extra_coords attached to a NDCube will reference a different NDCube?

No, that should be explicitly prohibited by the descriptor.

extra_coords will exist without a NDCube?

I think this might be possible yes.

ViciousEagle03 added 2 commits May 20, 2024 19:33

initial commit

39faa60

version alignment

9644bbb

ViciousEagle03 marked this pull request as draft May 20, 2024 15:58

braingram reviewed May 20, 2024

View reviewed changes

ViciousEagle03 added 10 commits May 23, 2024 04:16

Remove method to remove any delay and

af7dccb

Update tag and

67b8805

enable schema test

b62ac94

revert small change

e2928ea

Add the converters for ExtraCoords and TimeTableCoordinate class

c2b240f

Add ExtraCoords and TimeTableCoord Schema

94f2065

Update manifest and entry_point.py

cc65cc3

Add the QuantityTableCoordinate and SkyCoordTableCoordinate converters

5ffb493

Add the schema for QuantityTableCoordinate and SkyCoordTableCoordinate

695f8f6

Update manifest and entry_point.py

9e38352

ViciousEagle03 commented May 31, 2024

View reviewed changes

Comment thread ndcube/asdf/resources/schemas/ExtraCoords-0.1.0.yaml Outdated

Add validation for schema and manifests

a9c5868

nabobalis deleted the branch sunpy:asdf-support June 4, 2024 22:02

nabobalis closed this Jun 4, 2024

nabobalis reopened this Jun 4, 2024

nabobalis added this to the 2.4.0 milestone Jun 4, 2024

This comment was marked as outdated.

Sign in to view

nabobalis removed this from the 2.4.0 milestone Jun 4, 2024

Cadair added 2 commits June 5, 2024 11:48

Merge branch 'asdf-support' into asdf_basic_support

835c5c7

pre-commit

3bb70e4

Cadair added this to the 2.4.0 milestone Jun 5, 2024

Cadair added the No Changelog Entry Needed Skip all changelog checks. label Jun 5, 2024

Cadair reviewed Jun 5, 2024

View reviewed changes

Comment thread ndcube/asdf/converters/extracoords_converter.py

ViciousEagle03 added 2 commits June 5, 2024 21:37

Update the tox.ini and CI workflow

04409b5

Update schemas

76cc27e

ViciousEagle03 requested a review from braingram July 7, 2024 12:50

braingram suggested changes Jul 11, 2024

View reviewed changes

Comment thread ndcube/asdf/converters/tablecoord_converter.py Outdated

braingram reviewed Jul 11, 2024

View reviewed changes

Comment thread ndcube/asdf/resources/schemas/ndcube-0.1.0.yaml

braingram suggested changes Jul 11, 2024

View reviewed changes

ViciousEagle03 added 2 commits July 11, 2024 13:08

Remove mesh as a property validator

7c04d35

Update the dependencies version

266f88e

ViciousEagle03 commented Jul 11, 2024

View reviewed changes

Comment thread pyproject.toml Outdated

braingram approved these changes Jul 11, 2024

View reviewed changes

Cadair reviewed Jul 18, 2024

View reviewed changes

ViciousEagle03 added 3 commits July 20, 2024 00:51

Style changes and add warnings to NDCube converter

67358b4

env name update

6e7a338

Add asdf as an optional dep

c1a6b49

ViciousEagle03 commented Aug 5, 2024

View reviewed changes

Comment thread ndcube/asdf/converters/tests/test_ndcube_converter.py

Remove asdf as an optional dependency

0546cad

Cadair approved these changes Aug 5, 2024

View reviewed changes

Add support for meta in converter and schema

e072a9a

ViciousEagle03 mentioned this pull request Aug 16, 2024

Add support for the serialization of the ndcube WCS wrappers. #751

Merged

Add mask as a validator property

5cfa4f9

Cadair mentioned this pull request Aug 22, 2024

ExtraCoords half works if not attached to a NDCube #753

Open

Merge branch 'asdf-support' into asdf_basic_support

e8268fc

Cadair approved these changes Aug 22, 2024

View reviewed changes

Cadair merged commit 406b867 into sunpy:asdf-support Aug 22, 2024

Cadair modified the milestones: 2.4.0, asdf Aug 22, 2024

Uh oh!

Conversation

ViciousEagle03 commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR adds ASDF support for ndcube objects.

Uh oh!

braingram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

braingram left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

braingram left a comment

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 commented Jul 11, 2024

Uh oh!

Uh oh!

braingram left a comment

Choose a reason for hiding this comment

Uh oh!

Cadair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViciousEagle03 Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

braingram commented Jul 19, 2024

Uh oh!

ViciousEagle03 commented Aug 4, 2024

Uh oh!

nabobalis commented Aug 4, 2024

Uh oh!

ViciousEagle03 commented Aug 5, 2024

Uh oh!

Uh oh!

nabobalis commented Aug 5, 2024

Uh oh!

Cadair left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViciousEagle03 commented May 20, 2024 •

edited

Loading

braingram left a comment •

edited

Loading

ViciousEagle03 Jul 19, 2024 •

edited

Loading